RDK-61440: Implementation of handling PowerMode Change in NM plugin #308
RDK-61440: Implementation of handling PowerMode Change in NM plugin #308Anand73-n wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces PowerManager integration to handle DeepSleep-related power mode transitions in the NetworkManager plugin, while also improving thread-safety around default-interface handling and making several robustness fixes (iterator null checks, GLib/libnm context isolation, and deinit hooks). It also bumps the plugin/interface version to 2.2.0 and updates documentation/changelog accordingly.
Changes:
- Add a new
NetworkManagerPowerClient(COMRPC client) and hook it intoNetworkManagerImplementationto act on DeepSleep pre-change/changed events. - Make default-interface access thread-safe (
getDefaultInterface/setDefaultInterface) and update call sites across proxies/connectivity monitor. - Improve crash resilience/robustness (null checks on iterator creation, platform deinit functions, GNOME wifi operation serialization, minimal ethernet profile support) and update versioning/docs.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/l2Test/libnm/l2_test_libnmproxy.cpp | Minor unit test expectation cleanup / formatting fix. |
| plugin/rdk/NetworkManagerRDKProxy.cpp | Use thread-safe default-interface getters/setters; add iterator null checks; add platform_deinit(). |
| plugin/NetworkManagerPowerClient.h | New PowerManager client interface + callback contract for DeepSleep handling. |
| plugin/NetworkManagerPowerClient.cpp | New COMRPC client implementation with an internal worker thread and pre-change ack handling. |
| plugin/NetworkManagerJsonRpc.cpp | Add null checks for iterator creation before invoking implementation APIs. |
| plugin/NetworkManagerImplementation.h | Introduce thread-safe default-interface accessors, PowerManager callback integration, and platform deinit declaration. |
| plugin/NetworkManagerImplementation.cpp | Instantiate PowerManager client; call platform deinit in destructor; use thread-safe default-interface; implement power callbacks. |
| plugin/NetworkManagerConnectivity.cpp | Use thread-safe default-interface access in connectivity monitoring logic. |
| plugin/gnome/NetworkManagerGnomeWIFI.h | Add ethernet-minimal-profile helper; add mutex to serialize wifi operations. |
| plugin/gnome/NetworkManagerGnomeWIFI.cpp | Context push/pop moved to per-operation; serialize operations; add minimal ethernet profile; improve cleanup/error handling. |
| plugin/gnome/NetworkManagerGnomeProxy.cpp | Replace global NMClient with per-instance client/context; add deinit; add migration cleanup + minimal ethernet profile on eth enable. |
| plugin/gnome/gdbus/NetworkManagerGdbusProxy.cpp | Use thread-safe default-interface and add platform_deinit(). |
| plugin/CMakeLists.txt | Add ENABLE_POWERMANAGER option, build/link new power client when enabled. |
| legacy/LegacyWiFiManagerAPIs.cpp | Add null check after iterator creation. |
| legacy/LegacyNetworkAPIs.cpp | Add null check after iterator creation. |
| docs/NetworkManagerPlugin.md | Bump documented version to 2.2.0. |
| definition/NetworkManager.json | Bump interface version to 2.2.0. |
| CMakeLists.txt | Bump project version minor to 2 (2.2.0). |
| CHANGELOG.md | Add entries for 2.2.0 and 2.1.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(getDefaultInterface() != iface.name) | ||
| ReportActiveInterfaceChange(getDefaultInterface(), iface.name); |
| option(ENABLE_POWERMANAGER "Enable PowerManager COMRPC integration for DeepSleep WiFi management" ON) | ||
| if(ENABLE_POWERMANAGER) | ||
| find_package(${NAMESPACE}Definitions REQUIRED) | ||
| add_definitions(-DENABLE_POWERMANAGER) | ||
| message("NetworkManager: PowerManager integration enabled") | ||
| endif() |
| // If there are multiple messages backed up, process a bounded number | ||
| // of pending iterations so this path cannot stall indefinitely if the | ||
| // context keeps receiving new work. | ||
| while (g_main_context_iteration(device_context, FALSE)); |
Reason for change: Ctrl ntwrk state based on PowerMode transitions Test procedure: Change the PowerMode state and verify the behavior Risks: low Priority: P1 Signed-off-by: Anand N <Anand_N@comcast.com>
Reason for change: Add log to know cliendId Test procedure: Change the PowerMode and verify the log Risks: low Priority: P1 Signed-off-by: Anand N <Anand_N@comcast.com>
Reason for change: fix crash from power worker thread Test procedure: Change the PowerMode and verify the log Risks: low Priority: P1 Signed-off-by: Anand N <Anand_N@comcast.com>
Reason for change: offload prechange WiFi disc to dedicated powerthrd Test procedure: Change the PowerMode and verify the log Risks: low Priority: P1 Signed-off-by: Anand N <Anand_N@comcast.com>
Reason for change: enabled ConnectToKnownSSID() Test procedure: Change the PowerMode and verify the log Risks: low Priority: P1 Signed-off-by: Anand N <Anand_N@comcast.com>
Reason for change: trigger normal WiFi scan on DeepSleep wakeup with Network Standby ON to handle AP channel changes Test procedure: Change the PowerMode and verify the log Risks: low Priority: P1 Signed-off-by: Anand N <Anand_N@comcast.com>
0963fb3 to
afbdab5
Compare
Reason for change: check before initiating wifi connect and disconnect Test procedure: Change the PowerMode and verify the log Risks: low Priority: P1 Signed-off-by: Anand N <Anand_N@comcast.com>
| if (m_wlanEnabled.load() && !m_lastConnectedSSID.empty()) { | ||
| NMLOG_INFO("OnPowerModePreChange: waking from DeepSleep — reconnecting to '%s'", | ||
| m_lastConnectedSSID.c_str()); | ||
| ConnectToKnownSSID(m_lastConnectedSSID); // fire-and-forget |
Reason for change: Disconnect ethernet on DeepSleep entry and reconnect on wakeup OnPowerModePreChange Test procedure: Change the PowerMode and verify the log Risks: low Priority: P1 Signed-off-by: Anand N <Anand_N@comcast.com>
Reason for change: guard ethernet wakeup reconnect with m_ethDisconnectedForSleep Test procedure: Change the PowerMode and verify the log Risks: low Priority: P1 Signed-off-by: Anand N <Anand_N@comcast.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
plugin/NetworkManagerPowerClient.cpp:115
- Operational(false) stops and joins the power-event thread before unregistering from PowerManager. There is a race window where the COM-RPC dispatcher can still deliver an OnPowerModePreChange after the thread has exited, causing the notification handler to enqueue a PRE_CHANGE event that will never be processed/acked, potentially stalling the power-mode transition. Consider unregistering (or otherwise preventing further notifications / sending an immediate ack) before joining the thread, or making the notification path fast-ack when mStopThread is set.
} else {
// Stop the power-event thread before unregistering so any in-flight
// event that was already enqueued is drained with a fast ack.
mStopThread = true;
mQueueCv.notify_one();
if (mPowerThread.joinable()) {
mPowerThread.join();
}
unregisterEventsAndDeactivate();
}
| if (m_ethEnabled.load() && m_ethConnected.load()) { | ||
| NMLOG_INFO("OnPowerModePreChange: going to DeepSleep — disconnecting Ethernet"); | ||
| uint32_t rcEthDown = EthernetDisconnect(); | ||
| if (rcEthDown == Core::ERROR_NONE) { | ||
| m_ethDisconnectedForSleep.store(true); |
| option(ENABLE_POWERMANAGER "Enable PowerManager COMRPC integration for DeepSleep WiFi management" ON) | ||
| if(ENABLE_POWERMANAGER) | ||
| find_package(${NAMESPACE}Definitions REQUIRED) | ||
| add_definitions(-DENABLE_POWERMANAGER) | ||
| message("NetworkManager: PowerManager integration enabled") | ||
| endif() |
Reason for change: guard wifi wakeup reconnect with m_wlanDisconnectedForSleep Test procedure: Change the PowerMode and verify the log Risks: low Priority: P1 Signed-off-by: Anand N <Anand_N@comcast.com>
Reason for change: Additional logging and code alignment Test procedure: Change the PowerMode and verify the log Risks: low Priority: P1 Signed-off-by: Anand N <Anand_N@comcast.com>
Reason for change: Fix for trigger Wi-Fi scan OnPowerMode Changed Test procedure: Change the PowerMode and verify the log Risks: low Priority: P1 Signed-off-by: Anand N <Anand_N@comcast.com>
Do not merge.